Support for OpenBSD #688

wezm · 2017-07-18T21:50:30Z

Fixes #685

These changes get nix building on OpenBSD 6.1. There is one failing test that I want a little guidance on:

error[E0308]: mismatched types
   --> src/sys/event.rs:333:30
    |
333 |     assert!(expected.data == actual.data());
    |                              ^^^^^^^^^^^^^ expected i64, found isize

KEvent::data is:

    pub fn data(&self) -> intptr_t {
        self.kevent.data as intptr_t
    }

I assume the test should be updated to cast to the expected type but wanted to confirm that before making the change.

Susurrus · 2017-07-18T22:12:46Z

src/sys/event.rs

 pub type type_of_event_flag = u32;
 libc_bitflags!{
    pub flags EventFlag: type_of_event_flag {
        EV_ADD,
        EV_CLEAR,
        EV_DELETE,
        EV_DISABLE,
+        #[cfg(not(target_os = "openbsd"))]


We prefer to whitelist versus blacklist, this appears to be on mac, ios, dragonly, freebsd and netbsd.

Actually, OpenBSD does have it, see rust-lang/libc#613.

By the looks of it they aren't in a released version of OpenBSD yet. The author of that patch added it to the current branch but there hasn't been a release since. I think the linked PR is still unmerged as a result.

I think the right thing to do for now is to leave it out. I can raise another issue to add it when the next OpenBSD release occurs.

Sounds good. But please change this to a whitelist anyways. And add a TODO here that talks about adding it once OpenBSD is released (and name a specific version number so we don't need to look back in time to figure out this comment).

Susurrus · 2017-07-18T22:18:21Z

src/sys/signal.rs

@@ -490,11 +490,13 @@ pub enum SigevNotify {

 /// Used to request asynchronous notification of the completion of certain
 /// events, such as POSIX AIO and timers.
+#[cfg(not(target_os = "openbsd"))]


Is this because libc doesn't support this on openbsd or is it because it's unsupported on OpenBSD?

As far as I can tell it's not supported at all. No mention of sigevent in the man pages or headers.

Might be nicer than to wrap this in a module and #cfg that module and then a single line importing everything from it instead of having all of these #cfgs.

Susurrus · 2017-07-18T22:19:14Z

src/sys/socket/consts.rs

    pub const SO_TIMESTAMP: c_int           = libc::SO_TIMESTAMP;
+    #[cfg(target_os = "openbsd")]
+    pub const SO_TIMESTAMP: c_int           = 0x0800;


We don't hardcode values in nix anymore, this needs to be upstreamed into libc before it can be included here.

Susurrus · 2017-07-18T22:20:48Z

Regarding your specific question, yes it does appear that a cast is missing that'll use as type_of_data.

wezm · 2017-07-18T23:07:00Z

Thanks for the quick review.

Regarding your specific question, yes it does appear that a cast is missing that'll use as type_of_data.

So something like this?

assert!(expected.data as type_of_data == actual.data());

Susurrus · 2017-07-18T23:07:47Z

I think the other way around. Look at the lines of code around it and it'll be similar.

asomers · 2017-07-19T00:46:50Z

@wezm I would say that you should change the output type of the data function to type_of_data.

wezm · 2017-07-19T23:11:23Z

@Susurrus @asomers Happy to fix either way. Changing the return type of data would be an API change. Just want to confirm that's ok before going down that route.

asomers

On second thought I changed my mind. Right now, KEvent has the same interface on all OSes, which is a handy property. Let's do it the way you have it now.

wezm · 2017-07-20T11:33:36Z

Getting closer. All tests (excluding doc tests) are passing except this one:

---- sys::test_socket::test_scm_rights stdout ----
        thread 'sys::test_socket::test_scm_rights' panicked at 'Did not receive passed fd', /usr/obj/ports/rust-1.16.0/rustc-1.16.0-src/src/libcore/option.rs:715

test result: FAILED. 53 passed; 1 failed; 0 ignored; 0 measured

It's going to need bit of digging as the code looks right when compared to the example in CMSG_DATA(3) but there is a fair bit of unsafe work going on in ControlMessage::ScmRights.

Susurrus · 2017-07-20T13:44:20Z

src/sys/event.rs

 pub type type_of_event_flag = u32;
 libc_bitflags!{
    pub flags EventFlag: type_of_event_flag {
        EV_ADD,
        EV_CLEAR,
        EV_DELETE,
        EV_DISABLE,
+        #[cfg(not(target_os = "openbsd"))]


Sounds good. But please change this to a whitelist anyways. And add a TODO here that talks about adding it once OpenBSD is released (and name a specific version number so we don't need to look back in time to figure out this comment).

Susurrus · 2017-07-20T13:45:43Z

src/sys/signal.rs

@@ -490,11 +490,13 @@ pub enum SigevNotify {

 /// Used to request asynchronous notification of the completion of certain
 /// events, such as POSIX AIO and timers.
+#[cfg(not(target_os = "openbsd"))]


Might be nicer than to wrap this in a module and #cfg that module and then a single line importing everything from it instead of having all of these #cfgs.

Susurrus · 2017-07-20T13:46:40Z

test/test_unistd.rs

@@ -242,7 +242,7 @@ fn test_fpathconf_limited() {
 #[test]
 fn test_pathconf_limited() {
    // AFAIK, PATH_MAX is limited on all platforms, so it makes a good test
-    let path_max = pathconf(".", PathconfVar::PATH_MAX);
+    let path_max = pathconf("/", PathconfVar::PATH_MAX);


Why the change here?

See the commit message for that change. It was failing when the tests were run in parallel. Presumably one of them is doing something that makes . No longer exist.

good catch. There are indeed some tests that do that.

asomers · 2017-07-21T00:19:52Z

test/test_unistd.rs

@@ -242,7 +242,7 @@ fn test_fpathconf_limited() {
 #[test]
 fn test_pathconf_limited() {
    // AFAIK, PATH_MAX is limited on all platforms, so it makes a good test
-    let path_max = pathconf(".", PathconfVar::PATH_MAX);
+    let path_max = pathconf("/", PathconfVar::PATH_MAX);


good catch. There are indeed some tests that do that.

asomers · 2017-07-21T00:24:11Z

nix-test/src/const.c

@@ -209,6 +209,31 @@ get_int_const(const char* err) {
    GET_CONST(ECANCELED);
 #endif

+#if defined(__OpenBSD__)


You can revert the changes to this file. This file has been obsolete ever since most constants moved into libc. libc has its own, superior version.

I added these to get the errno tests passing. It looks like the check_errno macro uses them via assert_const_eq or did I miss something?

Grr, I thought we had already removed that. Go ahead and leave these changes in for now; we'll delete them in a future PR.

As we do conversions of our code to use upstream libc values, we can remove these tests incrementally until they're all gone. But for now, we should add these tests if we have any hard-coded values (like all the errno values currently are).

asomers

There are two remaining problems with this PR:

The hardcoded constant
The test_scm_rights failure on FreeBSD. It's repeatable.

asomers · 2017-07-23T22:00:02Z

nix-test/src/const.c

@@ -209,6 +209,31 @@ get_int_const(const char* err) {
    GET_CONST(ECANCELED);
 #endif

+#if defined(__OpenBSD__)


Grr, I thought we had already removed that. Go ahead and leave these changes in for now; we'll delete them in a future PR.

wezm · 2017-07-24T00:48:06Z

test_scm_rights failure on FreeBSD

Yep I'm working on this at the moment. It's proving quite a challenge to get all OSes working at once. Might take a little bit to get sorted but I am working on it a bit each day.

wezm · 2017-07-24T22:52:12Z

It was an adventure but I think 403cc5e finally resolves the failing control message test (for all platforms). As far as I can tell it was a bug in the implementation that only exposed itself on OpenBSD. More details in the commit message. Next up I'll deal with the other requested changes and get the doc tests passing. Not sure what's up with AlanCI failing to find the commit... but the tests pass on my FreeBSD VM.

asomers · 2017-07-25T00:47:23Z

Looks like the OSX build failure is trivial to fix. And don't forget to remove the hardcoded SO_TIMESTAMP value.

Add missing SO_TIMESTAMP constant for OpenBSD like OSes Added to mod.rs as both [OpenBSD](https://github.com/openbsd/src/blob/e35050970e3e2b822e383a5e844e4c4402eebccc/sys/sys/socket.h#L97) and [Bitrig](https://github.com/bitrig/bitrig/blob/a15267d8249fe8d329eb34048605c59a394a02ac/sys/sys/socket.h#L100) have it. Required for nix-rust/nix#688

Bump to version 0.2.29 Rationale for release: My changes in nix-rust/nix#688 are blocked on getting a libc release out with the missing constant present.

wezm · 2017-08-01T07:59:59Z

Ok folks, I think all requested changes have been addressed. I've replaced the SO_TIMESTAMP magic number with the one I upstreamed in libc. Should be good to merge now.

Susurrus · 2017-08-01T17:45:02Z

src/sys/signal.rs

@@ -3,7 +3,9 @@

 use libc;
 use {Errno, Error, Result};
+#[cfg(not(target_os = "openbsd"))]
 use std::fmt;


These can be combined into one use statement like use std::fmt::{self, Debug};.

This still isn't addressed.

Susurrus · 2017-08-01T17:47:13Z

src/sys/signal.rs

-    sigevent: libc::sigevent
-}
+#[cfg(not(target_os = "openbsd"))]
+pub mod sigevent {


I don't think this module should be public since you glob-import everything into the sys::signal module above.

Susurrus · 2017-08-01T17:51:12Z

You've got a couple of commits in there that I think should be merged with earlier ones as they're fixing problems that were created by this PR. You also have a merge commit there in the middle. I'd prefer if you rebased your changes rather then merged master back into your branch (makes it harder to read the history when there are a ton of merge commits on top of the ones bors adds).

This could also use a CHANGELOG entry if 0.9 didn't work on OpenBSD and the next release will.

I suggest you wait until #647 as it changes a bunch of socket constants.

wezm · 2017-08-02T11:35:00Z

Righto, addressed the comments on src/sys/signal.rs, rebased against current master and added a CHANGELOG entry.

Susurrus · 2017-08-02T15:09:37Z

src/sys/socket/ffi.rs

+
+#[cfg(not(target_os = "linux"))]
+pub type type_of_msg_iovlen = c_uint;
+


There should only be one empty line following this.

Susurrus · 2017-08-02T15:10:28Z

src/sys/socket/ffi.rs

@@ -33,9 +41,9 @@ pub struct msghdr<'a> {
    pub msg_name: *const c_void,


This struct should be replaced with the FFI types declared in libc, but that doesn't need to be done for this PR. Please add a FIXME stating that.

Susurrus · 2017-08-02T15:10:54Z

src/sys/socket/mod.rs

@@ -235,6 +235,7 @@ impl<'a> Iterator for CmsgIterator<'a> {

        // Advance our internal pointer.
        if cmsg_align(cmsg_len) > buf.len() {
+            println!("cmsg_align(cmsg_len) > buf.len(): {} > {}", cmsg_align(cmsg_len), buf.len());


I think this debugging statement should be removed, yes?

Susurrus · 2017-08-02T15:15:07Z

CHANGELOG.md

@@ -12,6 +12,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#672](https://github.com/nix-rust/nix/pull/672))
 - Added protocol families in `AddressFamily` enum.
  ([#647](https://github.com/nix-rust/nix/pull/647))
+- Added support for OpenBSD.


We didn't "add support". OpenBSD cannot be tested in CI yet, so we haven't added support here, but this PR does do is fix compilation and tests for OpenBSD targets.

Susurrus · 2017-08-02T15:16:09Z

Okay, my previous comments don't make sense if you look at the entire changelog because you have commits that fix issues that you introduce in previous commits. Can you squash 6870ee6 and 9e4e0c0 into one commit? I don't like having commits that introduce changes to have them later refined within the same PR. It makes browsing the git log after the PR is merged more difficult.

wezm · 2017-08-03T07:52:45Z

Thanks for your patience with me on this. I've more aggressively cleaned up and squashed commits to better stand on their own without subsequent changes. I changed the changelog message and added a FIXME to cmsg structs.

Susurrus

Yes, this is very close to being ready to merge here. Just a few small remaining issues.

Susurrus · 2017-08-03T15:16:59Z

src/sys/socket/mod.rs

@@ -168,7 +168,7 @@ unsafe fn copy_bytes<'a, 'b, T: ?Sized>(src: &T, dst: &'a mut &'b mut [u8]) {
 }


-use self::ffi::{cmsghdr, msghdr, type_of_cmsg_len, type_of_cmsg_data};
+use self::ffi::{cmsghdr, msghdr, type_of_cmsg_len, type_of_cmsg_data, type_of_msg_iovlen};


Can you alphabetize this list?

Susurrus · 2017-08-03T15:17:15Z

src/sys/socket/ffi.rs

 // Private because we don't expose any external functions that operate
 // directly on this type; we just use it internally at FFI boundaries.
 // Note that in some cases we store pointers in *const fields that the
 // kernel will proceed to mutate, so users should be careful about the
 // actual mutability of data pointed to by this structure.
+//
+// FIXME: Replace these structs with the ones defined in libc
+// E.g. https://doc.rust-lang.org/libc/x86_64-unknown-linux-gnu/libc/struct.msghdr.html


You can leave off this link.

Susurrus · 2017-08-03T15:20:25Z

src/sys/event.rs

@@ -79,16 +78,25 @@ pub enum EventFilter {
 }

 #[cfg(any(target_os = "macos", target_os = "ios",


Please alphabetize the order of the OSes since you're already modifying this attribute.

Susurrus · 2017-08-03T15:20:30Z

src/sys/event.rs

-#[cfg(any(target_os = "openbsd", target_os = "freebsd",
-          target_os = "dragonfly", target_os = "macos",
-          target_os = "ios"))]
+#[cfg(any(target_os = "freebsd", target_os = "dragonfly",


Please alphabetize the order of the OSes since you're already modifying this attribute.

Susurrus · 2017-08-03T15:22:40Z

src/sys/signal.rs

+    }
+
+    impl SigEvent {
+        // Note: this constructor does not allow the user to set the


Can you turn this into a proper doc comment since you're modifying this? This is definitely something that should be exposed to our end users rather than only visible when browsing the source.

Susurrus · 2017-08-03T15:25:32Z

@asomers Can you wrap up your review? I want to make sure all of your concerns are addressed before we merge.

asomers · 2017-08-03T15:55:15Z

The CHANGELOG entry is in the ADDED section, but that section should only be for new features. This ia bugfix commit, so its CHANGELOG entry should go into the FIXED section.

There appears to be some interaction with test_pathconf_limited and another one when they are run in parallel, causing it to return ENOENT so the path has been changed from . to /.

Also updates cmsg types to match structs on non-Linux OSes. The implementation of CmsgInterator did not correctly mirror the way the CMSG_FIRSTHDR and CMSG_NEXTHDR macros work in C. CMSG_FIRSTHDR does not attempt to align access since the pointer is already aligned due to being part of the msghdr struct. CmsgInterator was always aligning access which happened to work on all platforms except OpenBSD where the use of alignment was adding unexpected bytes to the expected size and causing the `cmsg_align(cmsg_len) > self.buf.len()` guard clause to return early.

wezm · 2017-08-05T11:03:57Z

All requested changes have been addressed.

wezm · 2017-08-08T21:00:21Z

Hi folks, could I trouble you for another review. Would love to get this merged.

asomers · 2017-08-08T21:06:05Z

All of my concerns are addressed, and I think susuruss's are too. I'll merge in 1 day if @Susurrus has no more objections.

Susurrus · 2017-08-09T15:30:49Z

bors r+

688: Support for OpenBSD r=Susurrus Fixes #685 These changes get nix building on OpenBSD 6.1. There is one failing test that I want a little guidance on: ``` error[E0308]: mismatched types --> src/sys/event.rs:333:30 | 333 | assert!(expected.data == actual.data()); | ^^^^^^^^^^^^^ expected i64, found isize ``` `KEvent::data` is: ``` pub fn data(&self) -> intptr_t { self.kevent.data as intptr_t } ``` I assume the test should be updated to cast to the expected type but wanted to confirm that before making the change.

bors · 2017-08-09T16:35:31Z

Build succeeded

Susurrus suggested changes Jul 18, 2017

View reviewed changes

wezm changed the title ~~Initial support for OpenBSD~~ Support for OpenBSD Jul 19, 2017

asomers approved these changes Jul 20, 2017

View reviewed changes

Susurrus reviewed Jul 20, 2017

View reviewed changes

asomers requested changes Jul 21, 2017

View reviewed changes

Susurrus mentioned this pull request Jul 21, 2017

Release 0.9.0 #632

Closed

10 tasks

asomers requested changes Jul 23, 2017

View reviewed changes

wezm mentioned this pull request Jul 25, 2017

Add missing SO_TIMESTAMP constant for OpenBSD like OSes rust-lang/libc#697

Merged

wezm mentioned this pull request Jul 31, 2017

Bump to version 0.2.29 rust-lang/libc#705

Merged

Susurrus suggested changes Aug 1, 2017

View reviewed changes

Susurrus suggested changes Aug 2, 2017

View reviewed changes

Susurrus suggested changes Aug 3, 2017

View reviewed changes

wezm added 4 commits August 5, 2017 15:04

Add OpenBSD compatibility to events

836c06d

Add OpenBSD compatibility to signals

0874cfa

Fix tests on OpenBSD

000366c

There appears to be some interaction with test_pathconf_limited and another one when they are run in parallel, causing it to return ENOENT so the path has been changed from . to /.

Add entry to CHANGELOG about OpenBSD fixes

0fbd223

asomers approved these changes Aug 8, 2017

View reviewed changes

Susurrus approved these changes Aug 9, 2017

View reviewed changes

bors bot merged commit 0fbd223 into nix-rust:master Aug 9, 2017


		#[cfg(not(target_os = "linux"))]
		pub type type_of_msg_iovlen = c_uint;

		@@ -33,9 +41,9 @@ pub struct msghdr<'a> {
		pub msg_name: *const c_void,

		@@ -79,16 +78,25 @@ pub enum EventFilter {
		}

		#[cfg(any(target_os = "macos", target_os = "ios",

Support for OpenBSD #688

Support for OpenBSD #688

Conversation

wezm commented Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 18, 2017

wezm commented Jul 18, 2017

Susurrus commented Jul 18, 2017

asomers commented Jul 19, 2017

wezm commented Jul 19, 2017

asomers left a comment

Choose a reason for hiding this comment

wezm commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wezm commented Jul 24, 2017 • edited by Susurrus Loading

wezm commented Jul 24, 2017

asomers commented Jul 25, 2017

wezm commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Aug 1, 2017

wezm commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Aug 2, 2017

wezm commented Aug 3, 2017 • edited Loading

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Aug 3, 2017

asomers commented Aug 3, 2017

wezm commented Aug 5, 2017

wezm commented Aug 8, 2017

asomers commented Aug 8, 2017

Susurrus commented Aug 9, 2017

bors bot commented Aug 9, 2017

Build succeeded

wezm commented Jul 18, 2017 •

edited

Loading

wezm commented Jul 24, 2017 •

edited by Susurrus

Loading

wezm commented Aug 3, 2017 •

edited

Loading